fix: align env-check with platform-aware CodeQL paths and fix parity test filtering#57
Conversation
…test filtering Remove stale install.path from codecome.yml so the platform-aware default (.tools/codeql/<plat>/current/codeql) takes over from config.py. Replace hardcoded env-check path with explicit platform checks covering old (current/) and new (osx64/, linux64/, win64/) install layouts. Add make env-check step after each make init in CI so the path check is exercised in PR builds. Add plugin.added, plugin.updated, connector.updated, and reference.updated to the parity-ignored event set in mock-llm-parity.py (renamed from _SERVE_ONLY_TYPES to _PARITY_IGNORED_TYPES). Simplify compare_events() to call normalize_event once per event via map. Add test_install_path_defaults_to_platform_specific config test.
📝 WalkthroughWalkthroughThis PR transitions CodeQL installation from explicit path references to managed installation, validates the setup across platform-specific directories, and optimizes mock LLM parity event filtering by consolidating ignored event types and improving normalization efficiency. ChangesCodeQL Managed Installation Integration
Mock LLM Parity Event Filtering Optimization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Coverage Report
Generated by pytest-cov on |
Greptile SummaryThis PR fixes two independent issues: (1) a stale hardcoded CodeQL install path in
Confidence Score: 5/5Safe to merge — all changes are additive fixes with no regressions introduced. The three change areas (YAML path removal, Makefile platform detection, parity filter expansion) are each narrow and well-tested. The No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[make init] -->|CODEQL_SKIP_INSTALL=1| B[write .tools/codeql/.disabled]
A -->|CODEQL=0| B
A -->|full install| C[install to .tools/codeql/PLATFORM/current/codeql]
D[make env-check] --> E{.tools/codeql/.disabled exists?}
E -->|Yes - skip| F[✅ CodeQL check skipped]
E -->|No - check| G{test -x CODEQL_BIN}
G -->|found| H[✅ Environment OK]
G -->|missing| I[❌ FAIL: run make init]
J[config.py resolve_config] --> K{CODEQL_INSTALL_PATH set?}
K -->|Yes| L[use env var path]
K -->|No| M{install.path in codecome.yml?}
M -->|Yes - was stale path| N[❌ old layout path]
M -->|No - after this PR| O[codeql_platform → osx64/linux64/win64]
O --> P[.tools/codeql/PLATFORM/current/codeql]
style N fill:#f99,stroke:#c00
style P fill:#9f9,stroke:#090
style H fill:#9f9,stroke:#090
style F fill:#9f9,stroke:#090
Reviews (2): Last reviewed commit: "fix: derive CodeQL binary path from unam..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tools/mock-llm-parity.py (1)
42-58: Clarify_SERVE_ONLY_TYPES“legacy alias” comment (and its usage)
- In tools/mock-llm-parity.py,
_SERVE_ONLY_TYPESis only defined at line 58 and used internally at line 345; no other in-repo references exist, so the line-57 comment about “external test references” is misleading—either clarify that it’s purely a backward-compat/internal alias, or switch the internal check to_PARITY_IGNORED_TYPESdirectly.- LGTM on the compare_events map/list-comprehension refactor (lines 413-414).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/mock-llm-parity.py` around lines 42 - 58, The comment claiming _SERVE_ONLY_TYPES is a "Legacy alias kept for external test references" is inaccurate because _SERVE_ONLY_TYPES is only defined here and only used internally (e.g., in the check that references _SERVE_ONLY_TYPES); either update the comment to state it's an internal/backwards-compatibility alias (mentioning the symbols _SERVE_ONLY_TYPES and _PARITY_IGNORED_TYPES) or remove the alias and change the internal usage to reference _PARITY_IGNORED_TYPES directly (so replace uses of _SERVE_ONLY_TYPES with _PARITY_IGNORED_TYPES and delete the alias definition).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_codeql_config.py`:
- Around line 55-71: The test test_install_path_defaults_to_platform_specific
must ensure the CODEQL_INSTALL_PATH env var is not set so resolve_config() uses
the platform default; before calling config_module.resolve_config(), remove or
unset CODEQL_INSTALL_PATH (use monkeypatch.delenv("CODEQL_INSTALL_PATH",
raising=False) or equivalent) to isolate the environment, then proceed to call
codeql.platform.codeql_platform() and config_module.resolve_config() and assert
on config.install_path.
---
Nitpick comments:
In `@tools/mock-llm-parity.py`:
- Around line 42-58: The comment claiming _SERVE_ONLY_TYPES is a "Legacy alias
kept for external test references" is inaccurate because _SERVE_ONLY_TYPES is
only defined here and only used internally (e.g., in the check that references
_SERVE_ONLY_TYPES); either update the comment to state it's an
internal/backwards-compatibility alias (mentioning the symbols _SERVE_ONLY_TYPES
and _PARITY_IGNORED_TYPES) or remove the alias and change the internal usage to
reference _PARITY_IGNORED_TYPES directly (so replace uses of _SERVE_ONLY_TYPES
with _PARITY_IGNORED_TYPES and delete the alias definition).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d6add3af-b38d-4661-aeb9-be976983523e
📒 Files selected for processing (6)
.github/workflows/tests.ymlMakefilecodecome.ymltests/test_codeql_config.pytests/test_mock_llm_parity.pytools/mock-llm-parity.py
💤 Files with no reviewable changes (1)
- codecome.yml
…nv, drop alias Replace multi-platform OR-chain in env-check with uname-derived CODEQL_PLATFORM (osx64/linux64/win64) and CODEQL_BIN. Accept CODEQL_INSTALL_PATH override as a make variable. Add monkeypatch.delenv for CODEQL_INSTALL_PATH in config test. Remove _SERVE_ONLY_TYPES alias, use _PARITY_IGNORED_TYPES directly in all call sites.
pruiz
left a comment
pruiz
left a comment
There was a problem hiding this comment.
Review feedback addressed in 08e0114
_SERVE_ONLY_TYPES alias (CodeRabbit nitpick on tools/mock-llm-parity.py):
Removed the alias entirely. normalize_event() now references _PARITY_IGNORED_TYPES directly. Zero callers remain referencing the old name.
CODEQL_INSTALL_PATH isolation (Greptile + CodeRabbit inline comments on tests/test_codeql_config.py):
Added monkeypatch.delenv("CODEQL_INSTALL_PATH", raising=False) at the top of test_install_path_defaults_to_platform_specific.
Makefile complexity (my own review finding about the multi-platform OR-chain):
Replaced the four-path OR-chain with uname -s detection via make variables:
UNAME_S := $(shell uname -s)CODEQL_PLATFORM— resolves toosx64/linux64/win64(with MINGW/MSYS/CYGWIN fallbacks)CODEQL_BIN := $(or $(CODEQL_INSTALL_PATH),.tools/codeql/$(CODEQL_PLATFORM)/current/codeql)
The env-check now tests exactly the host platform's managed binary path. No inline Python, no globs, no wildcards. Accepts CODEQL_INSTALL_PATH override as a make variable.
Docstring coverage warning (CodeRabbit pre-merge check):
Disregarded — not actionable for this PR.
Coverage comment showing 0/0 lines (GitHub Actions bot):
Pre-existing workflow issue unrelated to this PR.
Test results
- 815 passed, 0 failed (pytest)
- Frontmatter gate: 25 findings OK
make env-checkpasses on macOS (local) and all CI matrix jobs
1 participant
Summary
CodeQL env-check path fix
install.pathso the platform-aware default fromtools/codeql/config.pytakes effect (.tools/codeql/osx64/current/codeqlon macOS,linux64/on Linux)UNAME_S/CODEQL_PLATFORM/CODEQL_BINmake variables derived fromuname -s. The env-check tests exactly the host platform's managed binary path — no inline Python, no globs, no wildcard OR-chains. Also acceptsCODEQL_INSTALL_PATHoverride as a make variable.make env-checkafter eachmake initstep so the path check is exercised in PR buildstest_install_path_defaults_to_platform_specific, withCODEQL_INSTALL_PATHenv isolationParity test filtering fix
_SERVE_ONLY_TYPES→_PARITY_IGNORED_TYPES, addedplugin.added,plugin.updated,connector.updated,reference.updatedto the ignored set. Removed the old-name alias — all call sites reference_PARITY_IGNORED_TYPESdirectly. Simplifiedcompare_events()to callnormalize_event()once per event viamap.Why CI didn't catch the env-check bug
CI ran
make initthen directly invokedpytestandcheck-frontmatter.py, bypassingmake env-checkwhich is the prerequisite for allmaketargets likemake tests,make check, and all phases.Test results
make env-checkpasses on macOS (local) and all CI matrix jobs